Lint: reject a manifest naming an engine that is not configured - #105
Open
dudarenok-maker wants to merge 1 commit into
Open
Conversation
lint called a manifest naming a nonexistent engine clean, so the error cost a whole dispatch to discover: run then failed at spawn time, once the run row, the worktree and the dashboard entry already existed. Two holes, and the second is what made the first invisible. lint_manifest never resolved task.engine against the config. The only code that touches the name is noncanonical_route_findings, and an unknown name takes the quiet path out of it: config.engines.get returns None, model_key falls through to the identity registry's defaults, the route lookup misses and it continues. No finding, by construction. The lint CLI branch also returned before the shared AppConfig.load, so lint_manifest was always called with config=None - the parameter was already in its signature and simply never fed. A correct check alone would still have found nothing. That hole also degraded a check that already shipped: with config=None, noncanonical_route_findings falls back to the identity registry's defaults instead of the engine's real model_default for every task that does not name a model. Resolve each task's engine against config.engines and name the engines that are available, so the finding is diagnosable on its own. Load the config in the lint branch via the same suppressed --config argument run, hud, db, models and demo already take. If the config cannot be loaded, say so as the first finding rather than degrade quietly - a bare clean would claim more than was actually checked. Five tests, each guard exercised in both directions, because a guard only shown to block is indistinguishable from one welded shut, plus a wiring test that fails if the CLI goes back to config=None. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Burned a whole dispatch discovering a typo in an engine name.
lintsaidclean,runthen failed at spawn time — after the run row, the worktree andthe dashboard entry already existed. The manifest was generated by a tool of
mine, so the same wrong name would have come back every time until I read the
spawn error closely enough to recognise it as a name problem rather than a
missing binary.
lintis the cheap gate that exists to catch exactly this before a dispatch ispaid for, and on this input it certified nothing:
That manifest names
no-such-engine-xyz. It cannot run.Two holes, and the second is what made the first invisible
1.
lint_manifestnever resolvedtask.engineagainst the config. The onlycode that touches the name is
noncanonical_route_findings, and an unknown nametakes the quiet path out of it —
config.engines.get(task.engine)returnsNone,model_keyfalls through toidentity_registry.defaults.get(...)→"",noncanonical_routes.get((engine, ""))→None→continue(ringer.py6376-6382). No finding, by construction.
2. The
lintCLI branch returned before the sharedAppConfig.load. Thebranch ends at 11090/11092; the shared load is at 11097. So
lint_manifestwasalways called with
config=None— the parameter was already in its signatureand was simply never fed from the CLI. A correct check added in (1) alone would
still have found nothing.
That second hole also silently degraded a check that already shipped: with
config=None,noncanonical_route_findingsfalls back to the identityregistry's defaults instead of the engine's real
model_default, for every taskthat does not name a model explicitly.
What this does
config.engines, and reports an unknownone naming the engines that are available — so the finding is diagnosable on
its own, without a second round-trip to the config.
lintbranch, via the same suppressed--configargument
run,hud,db,modelsanddemoalready take (the main parsersupplies the default;
argparse.SUPPRESSon the subparser keeps it from beingclobbered). No new flag surface.
degrading quietly — a bare
cleanwould otherwise claim more than wasactually checked.
Same manifest, with this change:
On "one fix per PR" — this has an
and, deliberatelyThe check and the config wiring cannot be split into two mergeable PRs: the
check without the wiring is dead code that can never fire, and the wiring
without the check has no observable effect on its own. They are one defect —
lintruns configless — seen from two ends. Splitting would mean landing aprovably vacuous guard first, which is the failure mode the second half exists
to prevent.
Proof
tests/test_lint.pygains five tests, each guard exercised in bothdirections, because a guard only shown to block is indistinguishable from one
welded shut:
test_unknown_engine_is_reportedtest_configured_engine_is_not_reportedtest_check_is_vacuous_without_configconfig=Nonethe check cannot fire at alltest_cli_lint_loads_the_configconfig=Nonetest_cli_lint_stays_clean_on_a_configured_engineMutation-proved in both halves: neutering the check reddens two tests, dropping
the config load reddens one, restoring both goes green. The before/after above
is the real CLI against a real config, not a fixture.
Notes for review
a1a91b8, currentmain.concern (check parseability vs. engine identity). A textual conflict in
lint_manifest's findings block and intest_lint.py's import line islikely. Happy to rebase whichever lands second — no objection to Lint: reject a check no shell can parse #95 going
first.
resolvable") but at a different layer: Warn at config load when an engine bin is not resolvable — PATH problems currently surface mid-run #43 is a configured engine whose binary
will not resolve, this is a name that is not a configured engine at all. They
are complementary; neither catches the other's case.
tests/test_lint.pyhastwo failures on this box —
test_verifier_expands_user_expect_filesandtest_w6_write_collision— which reproduce identically on pristinea1a91b8(14 tests / 2 failures before, 19 tests / the same 2 after), sothey are pre-existing and path-shaped rather than anything this change did. I
make no POSIX claim beyond what the macOS and Linux jobs prove; the new tests
are stdlib-only, spawn no shell, and call
main()in-process withRINGER_NO_SELF_UPDATE=1set.operator wrote, not the worker — squarely "verify outputs", not confinement.